Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make push notifs configurable, i18n, & only for users missing labels #969

Merged
merged 1 commit into from
May 26, 2024

Conversation

JGreenlee
Copy link
Contributor

Make the text for push notifications configurable, translatable, and filterable to only include users with at least one recent trip that has no user input. The new config field 'push_notifications' has 'title' and 'message' (the text, per language) and 'recent_user_input_threshold' (the number of days to consider for determining 'recent user input'). Note the caveat about partially-labeled trips.

Make the text for push notifications configurable, translatable, and filterable to only include users with at least one recent trip that has no user input.
The new config field 'push_notifications' has 'title' and 'message' (the text, per language) and 'recent_user_input_threshold' (the number of days to consider for determining 'recent user input').
Note the caveat about partially-labeled trips.
@JGreenlee
Copy link
Contributor Author

I believe this works, but I don't know a good way to test it end-to-end. I thought I could load a dump (of stage, including the push_token_mapping), run the script, and it would go to our test phones.
But none of the dumps I have are recent enough to include any trips in the past week, so no users would be notified.

@JGreenlee
Copy link
Contributor Author

users_without_recent_user_input and bin_users_by_lang are reusable functions, which could potentially be used in other parts of the codebase. I think they should be moved somewhere else, I just don't know the best place to put them

@JGreenlee JGreenlee marked this pull request as ready for review May 24, 2024 13:58
@shankari
Copy link
Contributor

I believe this works, but I don't know a good way to test it end-to-end. I thought I could load a dump (of stage, including the push_token_mapping), run the script, and it would go to our test phones.
But none of the dumps I have are recent enough to include any trips in the past week, so no users would be notified.

I can generate a new dump on staging but that still won't be enough, because you don't have the API key that allows you to send the notifications. The only option would be for me to pull it and run it directly against staging. Trying that now...

JGreenlee added a commit to JGreenlee/nrel-openpath-deploy-configs that referenced this pull request May 24, 2024
Copy link
Contributor

@shankari shankari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

wrt

I think they should be moved somewhere else, I just don't know the best place to put them

This would be emission/core/common.py https://github.com/e-mission/e-mission-server/blob/master/emission/core/common.py

I will merge this as soon as I test on staging; should be out by the end of the weekend.

import json
import logging
import os
import requests
import sys
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops! I guess it was still exiting if the config wasn't available, but with a different error?!

import emission.net.ext_service.push.notify_usage as pnu

STUDY_CONFIG = os.getenv('STUDY_CONFIG', "stage-program")


def users_without_recent_user_input(uuid_list, recent_user_input_threshold=None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

future fix: it should be possible to write unit tests for both these function, particularly if we want to reuse them later.

return filtered_uuids


def bin_users_by_lang(uuid_list, langs, lang_key='phone_lang'):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

@shankari
Copy link
Contributor

To test this, I modified stage-program to have a custom label and message
e-mission/nrel-openpath-deploy-configs@317d991

I then tested as below:

  • without these changes
$ ./e-mission-py.bash bin/push/push_remind.py`
DEBUG:root:{'ios': {'multicast_ids': [7491960055826070491], 'success': 19, 'failure': 8, 'canonical_ids': 0, 'results': [{'message_id': '1716656656289564'}, {'message_id': '1716656656289622'}, {'message_id': '1716656656289623'}, {'message_id': '1716656656289974'}, {'message_id': '1716656656289975'}, {'message_id': '1716656656290406'}, {'message_id': '1716656656290408'}, {'message_id': '1716656656290648'}, {'error': 'NotRegistered'}, {'message_id': '1716656656290649'}, {'message_id': '1716656656291003'}, {'error': 'NotRegistered'}, {'message_id': '1716656656291007'}, {'message_id': '1716656656291008'}, {'error': 'NotRegistered'}, {'message_id': '1716656656291486'}, {'message_id': '1716656656291649'}, {'error': 'NotRegistered'}, {'message_id': '1716656656291663'}, {'error': 'NotRegistered'}, {'error': 'NotRegistered'}, {'error': 'NotRegistered'}, {'error': 'NotRegistered'}, {'message_id': '1716656656338761'}, {'message_id': '1716656656339299'}, {'message_id': '1716656656472074'}, {'message_id': '1716656656356078'}], 'topic_message_id': None}, 'android': {'multicast_ids': [1208182190971317416], 'success': 16, 'failure': 31, 'canonical_ids': 0, 'results': [{'error': 'NotRegistered'}, {'error': 'NotRegistered'}, {'error': 'NotRegistered'}, {'error': 'NotRegistered'}, {'message_id': '0:1716656656087748%3bcd12aef9fd7ecd'}, {'error': 'NotRegistered'}, {'message_id': '0:1716656656083857%3bcd12aef9fd7ecd'}, {'message_id': '0:1716656656083726%3bcd12aef9fd7ecd'}, {'error': 'NotRegistered'}, {'error': 'NotRegistered'}, {'error': 'NotRegistered'}, {'message_id': '0:1716656656085024%3bcd12aef9fd7ecd'}, {'error': 'NotRegistered'}, {'error': 'NotRegistered'}, {'error': 'NotRegistered'}, {'message_id': '0:1716656656088261%3bcd12aef9fd7ecd'}, {'message_id': '0:1716656656085591%3bcd12aef9fd7ecd'}, {'error': 'NotRegistered'}, {'error': 'NotRegistered'}, {'message_id': '0:1716656656084609%3bcd12aef9fd7ecd'}, {'error': 'NotRegistered'}, {'message_id': '0:1716656656091182%3bcd12aef9fd7ecd'}, {'error': 'NotRegistered'}, {'message_id': '0:1716656656095435%3bcd12aef9fd7ecd'}, {'message_id': '0:1716656656100514%3bcd12aef9fd7ecd'}, {'error': 'NotRegistered'}, {'error': 'NotRegistered'}, {'message_id': '0:1716656656104025%3bcd12aef9fd7ecd'}, {'error': 'NotRegistered'}, {'error': 'NotRegistered'}, {'error': 'NotRegistered'}, {'error': 'NotRegistered'}, {'error': 'NotRegistered'}, {'error': 'NotRegistered'}, {'error': 'NotRegistered'}, {'message_id': '0:1716656656124747%3bcd12aef9fd7ecd'}, {'error': 'NotRegistered'}, {'error': 'NotRegistered'}, {'error': 'NotRegistered'}, {'error': 'NotRegistered'}, {'error': 'NotRegistered'}, {'message_id': '0:1716656656134259%3bcd12aef9fd7ecd'}, {'error': 'NotRegistered'}, {'message_id': '0:1716656656133615%3bcd12aef9fd7ecd'}, {'message_id': '0:1716656656134255%3bcd12aef9fd7ecd'}, {'message_id': '0:1716656656134665%3bcd12aef9fd7ecd'}, {'error': 'NotRegistered'}], 'topic_message_id': None}}
  • with these changes, but with no recent trips on my phone, only two notifications were sent, and none to me
$ STUDY_CONFIG=stage-program ./e-mission-py.bash bin/push/push_remind.py
DEBUG:root:{'ios': {'multicast_ids': [528428971104671253], 'success': 2, 'failure': 0, 'canonical_ids': 0, 'results': [{'message_id': '1716656522603074'}, {'message_id': '1716656522603113'}], 'topic_message_id': None}, 'android': {'multicast_ids': [2057606944659572513], 'success': 2, 'failure': 0, 'canonical_ids': 0, 'results': [{'message_id': '0:1716656522413230%3bcd12aef9fd7ecd'}, {'message_id': '0:1716656522414481%3bcd12aef9fd7ecd'}], 'topic_message_id': None}}
  • with these changes, but with recent trips on my phone, three notifications were sent, one was to me and it was with custom text
$ STUDY_CONFIG=stage-program ./e-mission-py.bash bin/push/push_remind.py
DEBUG:root:{'ios': {'multicast_ids': [4869770820430893907], 'success': 3, 'failure': 0, 'canonical_ids': 0, 'results': [{'message_id': '1716735727883308'}, {'message_id': '1716735727884027'}, {'message_id': '1716735727884025'}], 'topic_message_id': None}, 'android': {'multicast_ids': [2396205895148880234], 'success': 2, 'failure': 0, 'canonical_ids': 0, 'results': [{'message_id': '0:1716735727769703%3bcd12aef9fd7ecd'}, {'message_id': '0:1716735727769553%3bcd12aef9fd7ecd'}], 'topic_message_id': None}}
Without changes With changes and with trips
IMG_0890 IMG_0895

@shankari shankari merged commit 3150c99 into e-mission:master May 26, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Address code reviews with "future"
Development

Successfully merging this pull request may close these issues.

2 participants